-
Notifications
You must be signed in to change notification settings - Fork 55
feat: support retry in health-check #131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
a6c14c2
to
3daee06
Compare
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
end | ||
|
||
self.init_count = (self.init_count or 0) + 1 | ||
local pos = self.init_count % endpoints_len + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intended to skip the first endpoint? As the minimal init_count here is 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also failed to figure out the purpose of this. It's the implementation of the earliest v3 API.
Hi @nic-chen do you remember why is it set so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about self.init_count = (self.init_count or -1) + 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don’t remember, it’s contributed by the community
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Fixed as suggested. Thanks!
lib/resty/etcd/v3.lua
Outdated
elseif body.error and body.error.http_code >= 500 then | ||
if health_check.conf ~= nil then | ||
-- health_check retry should do nothing here | ||
-- and let connection broke to create a new one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
broke => broker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I mean closed 🤣 fixed
return _request_uri(self, endpoint, "GET", | ||
endpoint.http_host .. "/v2/stats/self", | ||
nil, self.timeout) | ||
return _request_uri(self, "GET", "/v2/stats/self", nil, self.timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not about this PR: is there /v2
API in the /v3
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for API exists in both version, like /get
and /put
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
…oint choose problem in etcd (https://github.com/api7/lua-resty-etcd/pull/131\#discussion_r655804238) Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu wosoyoung@gmail.com
retry
in health-check. When it is enabled, operations would keep trying the same endpoint till unhealthy, and then other endpoints, until success or no endpoints healthy. So we could ensure each operation would send to etcd with best effort.Currently not support for
read_watch
since it would be better to close the original connection and create a new watch with a new endpoint.choose_enpoint
intorequest_uri
andrequest_chunk
so it could find alternative endpoint easier